-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Yap Jia Aun] Duke Increments #351
base: master
Are you sure you want to change the base?
Conversation
Add toolVersion block in to Gradle code sample to prevent errors.
Change file mode on `gradle` to be executable (nus-cs2103-AY1920S1#9)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, great job! I think OOP design is pretty solid so far and things are pretty clean, besides minor issues with style, you did a great job imo. LGTM! 👍
* Represents user's 'deadline' commmand to chatbot. | ||
* The 'DeadlineCommand' class supports operators (i) executing the command | ||
* and (ii) checking if the bot has exited its conversation with the user(in superclass). | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very thorough javadoc descriptions!
try { | ||
Date twentyFourHourFormat = new SimpleDateFormat("dd/MM/yyyy HHmm").parse(deadline); | ||
String twelveHourFormat = new SimpleDateFormat("dd/MM/yyy hh:mm").format(twentyFourHourFormat); | ||
this.deadline = new SimpleDateFormat("dd/MM/yyy hh:mm").parse(twelveHourFormat); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps another way of doing it would be to just use:
this.deadline = twentyFourHourFormat;
This prevents the need for that additional parse back from twelveHourFormat back to the Date object again.
|
||
/** | ||
* Executes the command and print out respecive reponse. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small typos here haha.
* Excutes the command and print out respective response.
* @return boolean of whether program has exited | ||
*/ | ||
public abstract boolean isExit(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding newlines at the end of your files, as it is a general convention. It doesn't really cause any issues beside making GitHub and checkstyle whine, but there is a reason behind doing so, https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline. Hope this gives some insight!
taskList.markTaskDone(taskNum); | ||
Task updatedTask = taskList.getTask(taskNum); | ||
storage.updateText(taskNum); | ||
storage.updateText(taskNum); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why updateText here is called twice?
|
||
default: | ||
return determineInputType(input); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is neat! I didn't think of separating input commands vs. bye and list commands. 👍
*/ | ||
public static Command determineInputType(String input) throws DukeException { | ||
if (input.contains("done")) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be careful with using input.contains() because for cases where the input is, for example:
todo select deadline for goals
Or conversely:
deadline do up a todo list /by 12/12/19 12:30
Your code might then not work as intended. You can consider instead using the startsWith() function (https://www.tutorialspoint.com/java/java_string_startswith.htm). 😃
/** | ||
* List of tasks. | ||
*/ | ||
private ArrayList<Task> todoList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small semantic nitpick but a todoList might imply a list of todo tasks instead a list of tasks. I totally understand this however. 😅
*/ | ||
public String getStatusIcon() { | ||
return (this.isDone ? "v" : "x"); // "\u2713" : "\u2718"); //return tick or X symbols | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned previously in my reply to your comments, if the alt codes there don't work do let me know! I'm curious as to why it doesn't work as well. 😃
A-Assertions updates before merge
A-CodeQuality updates
Merge master with branch-A-Streams
Pull from remote repository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general very clean code and easy to read, good job!
Duke Increments